Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept doubles (microseconds) in JWT timestamps when verifying claims. #282

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mathiasstocker
Copy link

Hi there

When implementing your library we noticed that tokens fail verification.
After some debugging we noticed, that the iobucci/jwt library issues timestamps in milliseconds (double) which fails your claim verification which expects an integer.

There was a discussion at the iobucci/jwt library regarding this topic ending in a wontfix.

RFC7519 says

NumericDate
A JSON numeric value representing the number of seconds from
1970-01-01T00:00:00Z UTC until the specified UTC date/time,
ignoring leap seconds. This is equivalent to the IEEE Std 1003.1,
2013 Edition [POSIX.1] definition "Seconds Since the Epoch", in
which each day is accounted for by exactly 86400 seconds, other
than
that non-integer values can be represented. See RFC 3339
[RFC3339]
for details regarding date/times in general and UTC in
particular.

In our opinion the definition is not clear as it not says explicit the seconds timestamp MUST be an integer.
It can be read as implicit definition of an integer if "other than that non-integer values" is meant regarding RFC 3339 which is mostly about Date/Time representation as string.

This PR alters your claim verification by allowing integer and double as type for the timestamps.

List of common tasks a pull request require complete

  • [] Changelog entry is added or the pull request don't alter library's functionality

* $nbf instead of $exp in not before check.
* Parenthesis setting corrected
@azmeuk
Copy link
Collaborator

azmeuk commented Dec 24, 2021

Hi. Thank you for your contribution. I am ok with the patch. Would you consider writing a simple unit test for it?

@mathiasstocker
Copy link
Author

Hello @azmeuk
I would like to write a test but i don't see how.
As far as i can see, the verifyJWTclaims function is protected and is called from the authenticate function.
In the existing token verification test only the verifyJWTsignature function is tested.

If you will accept that the verifyJWTclaims function is public then i can provide a test for it, otherwise i will need to mock all the other stuff in the authenticate function.

@dunglas
Copy link
Contributor

dunglas commented Mar 29, 2022

The patch I proposed here should also fix this issue: #287 (comment)

Delegating to a third-party JWT library (I suggest using web-token/jwt-core) instead of having an ad-hoc implementation would help have better JWT support.

@DeepDiver1975
Copy link
Collaborator

Delegating to a third-party JWT library (I suggest using web-token/jwt-core) instead of having an ad-hoc implementation would help have better JWT support.

we should do a close analysis which jwt lib to use - there is also firebase/php-jwt ....

@dunglas
Copy link
Contributor

dunglas commented Mar 30, 2022

My 2 cents: I did just that this week because I'm writing a library implementing the Solid OIDC specification to be released next week during SymfonyLive.

I tried to implement Solid OIDC using Lcobucci JWT, Firebase JWT, and finally JWT Framework. As of today, JWT Framework is the only one supporting all features required by Solid OIDC (including OAuth DPoP): JWK, JWKS and ECDSA.

@DeepDiver1975
Copy link
Collaborator

closes #298

@omitobi
Copy link

omitobi commented Nov 24, 2023

Is there still a plan to fix this issue?

@@ -1027,8 +1027,8 @@ protected function verifyJWTclaims($claims, $accessToken = null) {
return (($this->issuerValidator->__invoke($claims->iss))
&& (($claims->aud === $this->clientID) || in_array($this->clientID, $claims->aud, true))
&& (!isset($claims->nonce) || $claims->nonce === $this->getNonce())
&& ( !isset($claims->exp) || ((gettype($claims->exp) === 'integer') && ($claims->exp >= time() - $this->leeway)))
&& ( !isset($claims->nbf) || ((gettype($claims->nbf) === 'integer') && ($claims->nbf <= time() + $this->leeway)))
&& ( !isset($claims->exp) || ((gettype($claims->exp) === 'integer' || gettype($claims->exp) === 'double') && ($claims->exp >= time() - $this->leeway)))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cleaner to use in_array for this. As example:

(in_array(gettype($claims->exp), ['double', 'float', 'integer']) && ($claims->exp >= time() - $this->leeway)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants